Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

skybian defaults from skywire-cli visor gen-config -s #852

Merged
merged 12 commits into from
Aug 24, 2021
Merged

skybian defaults from skywire-cli visor gen-config -s #852

merged 12 commits into from
Aug 24, 2021

Conversation

0pcom
Copy link
Collaborator

@0pcom 0pcom commented Jul 24, 2021

Did you run make format && make check?
yes. There were some errors that appeared to be pre-existing, with make check

Changes:

  • Adds the -s flag to skywire-cli visor gen-config for generating config files with the defaults for skybian.

This is useful in the event the config files are messed up, correct config defaults can be re-generated on the running board without the need for skyimager or reflashing the image onto the microSD card.

  • Streamlines the behavior of the -p and -s flags by combining replace and setting output to the correct / expected path (used by the systemd service)

In a typical instance of configs generated by postinstall scripts of a package, the configuration for the hypervisor was previously accomplished by the following:

cd /opt/skywire
skywire-cli visor gen-config --is-hypervisor -pro /opt/skywire/skywire.json

with this PR, the same output as the above is accomplished with:

skywire-cli visor gen-config -ip

Peviously it was not possible to reproduce the configuration file generated by skyimager.
Generating the skybian config file:
Hypervisor

skywire-cli visor gen-config -is

Visor

skywire-cli visor gen-config -s

example output:

$ sudo go run skywire-cli.go visor gen-config -is
[2021-07-24T15:57:49-05:00] INFO [visor:config]: Flushing config to file. config_version="v1.1.0" filepath="/etc/skywire-config.json"
[2021-07-24T15:57:49-05:00] INFO [visor:config]: Flushing config to file. config_version="v1.1.0" filepath="/etc/skywire-config.json"
[2021-07-24T15:57:49-05:00] INFO [skywire-cli]: Updated file '/etc/skywire-config.json' to: {
	"version": "v1.1.0",
	"sk": "9d7e69eb96e462101bf479505819425c5902894a23f792b2e477072bfdd3ff5a",
	"pk": "035887660f00d5edabc1282228425a70c02171c2b5004f747e2b8e4c94cbb7e9ae",
	"dmsg": {
		"discovery": "http://dmsgd.skywire.skycoin.com",
		"sessions_count": 1
	},
	"dmsgpty": {
		"port": 22,
		"cli_network": "unix",
		"cli_address": "/run/skywire-visor/dmsgpty/cli.sock"
	},
	"stcp": {
		"pk_table": null,
		"local_address": ":7777"
	},
	"transport": {
		"discovery": "http://tpd.skywire.skycoin.com",
		"address_resolver": "http://ar.skywire.skycoin.com",
		"public_autoconnect": false,
		"transport_setup_nodes": null
	},
	"routing": {
		"setup_nodes": [
			"0324579f003e6b4048bae2def4365e634d8e0e3054a20fc7af49daf2a179658557"
		],
		"route_finder": "http://rf.skywire.skycoin.com",
		"route_finder_timeout": "10s",
		"min_hops": 0
	},
	"uptime_tracker": {
		"addr": "http://ut.skywire.skycoin.com"
	},
	"launcher": {
		"discovery": {
			"update_interval": "1m0s",
			"proxy_discovery_addr": "http://sd.skycoin.com"
		},
		"apps": [
			{
				"name": "skychat",
				"args": [
					"-addr",
					":8001"
				],
				"auto_start": true,
				"port": 1
			},
			{
				"name": "skysocks",
				"auto_start": true,
				"port": 3
			},
			{
				"name": "skysocks-client",
				"auto_start": false,
				"port": 13
			},
			{
				"name": "vpn-server",
				"auto_start": false,
				"port": 44
			},
			{
				"name": "vpn-client",
				"auto_start": false,
				"port": 43
			}
		],
		"server_addr": "localhost:5505",
		"bin_path": "/usr/bin/apps"
	},
	"hypervisors": [],
	"cli_addr": "localhost:3435",
	"log_level": "info",
	"local_path": "/var/skywire-visor/apps",
	"shutdown_timeout": "10s",
	"restart_check_delay": "1s",
	"is_public": false,
	"hypervisor": {
		"db_path": "/var/skywire-visor/users.db",
		"enable_auth": true,
		"cookies": {
			"hash_key": "a0f669a79e5d5ba8ac284cbef9656198cde7a1772884233b8f7a99426ca7e945c5f77b47994987272e0b8fce5fa878f77eb111da7554d1431160856335a669d3",
			"block_key": "1e4a4dd0518b2090b4dc9fe72b150194222ef1e51e735f88e8121dcbac14396b",
			"expires_duration": 43200000000000,
			"path": "/",
			"domain": ""
		},
		"dmsg_port": 46,
		"http_addr": ":8000",
		"enable_tls": false,
		"tls_cert_file": "/var/skywire-visor/ssl/cert.pem",
		"tls_key_file": "/var/skywire-visor/ssl/key.pem"
	}
}

@0pcom
Copy link
Collaborator Author

0pcom commented Jul 26, 2021

Ive fixed the make check errors, thanks to @mrpalide

Its unclear to me what the cookie path specification is for, pertaining to the hypervisor. Should it be /? I should think not if it is writing anything in /

I will also note that for the skybian paths / envs, should consider importing them from some file in skybian instead of hardcoding them as ive done. In order that they can be set in the skybian repo and that setting will be reflected in skywire when importing a go package containing these constants.

jdknives
jdknives approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright.

mrpalide
mrpalide approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@mrpalide mrpalide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work.

@ersonp
Copy link
Contributor

ersonp commented Jul 28, 2021

Everything looks good except for one thing. I think all of these commands should use the -r flag if the config is already present in that location.

skywire-cli visor gen-config -ip
skywire-cli visor gen-config -is
skywire-cli visor gen-config -s

@mrpalide
Copy link
Contributor

Everything looks good except for one thing. I think all of these commands should use the -r flag if the config is already present in that location.

skywire-cli visor gen-config -ip
skywire-cli visor gen-config -is
skywire-cli visor gen-config -s

Automatically will set, In L64 or in L72

@ersonp
Copy link
Contributor

ersonp commented Jul 29, 2021

I meant that we are normally using a -r flag so I think for consistency, we should either have it for all or not have it for all. What do you think @jdknives @0pcom @mrpalide ?

@0pcom
Copy link
Collaborator Author

0pcom commented Aug 2, 2021

It's the intended action for the -p and -s flags to be able to create and replace specific configs.

I am not opposed to the removal of the -r flag and making it default behavior.

jdknives
jdknives reviewed Aug 4, 2021
View reviewed changes
Copy link
Member

@jdknives jdknives left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ersonp @0pcom did any of you test this on an actual board? Overall good PR. Thanks.

@0pcom Please document the usage with -r flag and do not automatically set it.

cmd/skywire-cli/commands/visor/gen-config.go Show resolved Hide resolved
pkg/skyenv/values.go Outdated Show resolved Hide resolved
pkg/skyenv/values.go Outdated Show resolved Hide resolved
@ersonp
Copy link
Contributor

ersonp commented Aug 5, 2021

@jdknives No, I don't have access to a compatible board.

@0pcom
Copy link
Collaborator Author

0pcom commented Aug 11, 2021

This should be ready now with the requested changes

@0pcom
Copy link
Collaborator Author

0pcom commented Aug 20, 2021

did any of you test this on an actual board?

I will make a package which reflects the skybian installation of skywire which uses -s to generate the configs. I'm using -p with current packages and it works just fine; however ther are several changes I've made which need to be reflected in the install or postinstall scripts.. Changing the link that is printed to http from https is the only one that comes to mind at the moment.

hopefully this gets merged soon and I can make those changes.

@jdknives jdknives merged commit 5345fdd into skycoin:develop Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ersonp ersonp ersonp left review comments

@jdknives jdknives jdknives approved these changes

@mrpalide mrpalide mrpalide approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0pcom @ersonp @mrpalide @jdknives